For this particular function, my goal was to actually move its call to BlockEdit or something and add it to the context of a block (probably as a private thing), that way it's only called once per block (as opposed to one per hook like today).

This can totally be shipped as a separate PR regardless of how we decide to move forward.

Pushed a commit that fixes all the typos in the docblocks.

Does it make sense to move useSettings() to a separate file? Perhaps under ../use-settings/index.js? Right now it's a bit unfortunate that use-setting/index.js exports useSettings().

The hooks are very closely related, it's not justified to have them in separate files.

Most consumers should import them from the parent components module, not using the /use-setting subpath.

I disagree because I still find it confusing and unexpected, but I'll agree to disagree and commit.

Maybe we should keep the same name as the subsetting name:

Here I chose the variable name based on the name of the setting: customFontSize.

I may be missing something, but that's exactly why I added the suggestion - in the PR we're using customFontSizes and not customFontSize.

Ah, I see 🙂 Changed as suggested.

I think it could be nice to consider building an ESLint rule that prevents folks from having multiple useSetting calls at the same level, recommending that we use useSettings() instead.

Not for this PR, obviously, but something that we discussed earlier and I wanted to make sure it doesn't fall through the cracks.

what's the purpose of this useMemo? am I mistaken in that it's doing nothing since all the internal function does is return its stored value?

On each useSettings call, the paths array will be different, although it contains the same values. This useMemo is to ensure that paths is a constant in that case, and to ensure that useSelect can memoize its return value. useSelect itself does something similar internally.

aha! the second value is the dependencies array, which in this case is paths itself. in my head I was reading [paths]

it would be nice to have a docblock-style comment explaining the purpose of this function, and I wonder if the name could be clearer since mergeOrigins doesn't outwardly communicate that there's a cache involved.

I'm also confused on the value we're storing here, or why we need flatMap(). In the old code it's not clear to me either, so maybe this is why 🤷‍♂️ but it looks like it might be using .concat() with a default of [] to avoid needing a conditional for the case where result is missing the necessary key. I guess it's convenient for when keys are undefined and then it automatically returns an array with only those values.

but the old code was directly returning these things too while here we're storing them. since these are retrieved from value is there a need to split them out at this point? did you find that extracting these three properties was showing up in the hot path?

I'm so confused on all of this, really. is it worth adding a separate function and cache vs. directly building the array every time? maybe it was all confused by the nature of the original use of reduce()? I'm not trying to argue that the cache isn't worth it; I'm only asking if we have reason to believe it is.

const values = [];
[ 'default', 'theme', 'custom' ].forEach(
	( key ) => {
		if ( value[ key ] ) {
			values.push( value[ key ] );
		}
	}
)

return values;

Some settings, like color.palette, have a "raw" value where settings from different "origins" are in separate arrays:

{
  default: [ ... ],
  theme: [ ... ],
  custom: [ ... ],
}

The mergeOrigins function merges the three arrays into one, concatenating them together. I added memoization so that two calls to useSelect with the same state and arguments return the same value. This was a little pre-existing bug.

I added a docblock to the function that contains this explanation.

did you consider using variable argument counts here? it seems like most cases we'd be calling this we'll have literal values vs. dynamically-created arrays and I wonder if that's a convenience that would be nice.

const [ … ] = useSettings( 'setting-a', 'setting-b', 'color.setting.thing' );

Yes, that's a good idea. Variable arguments are better than an array argument. This is quite related to @tyxla's feedback about the module name:

Does it make sense to move useSettings() to a separate file? Perhaps under ../use-settings/index.js? Right now it's a bit unfortunate that use-setting/index.js exports useSettings().

To solve both problems, we could do this:

1. Rewrite useSettings to use variable arguments.
2. Migrate all remaining usages of useSetting to useSettings. The entire difference is the return value as array: const [ a ] = useSettings( 'a' ) vs const a = useSetting( 'a' ).
3. This makes useSetting practically deprecated: there's no good reason any more to use it instead of useSettings.
4. The actual source file can then be renamed to use-settings/index.js. The main thing it exports is useSettings, and then there's also the little useSetting back-compat wrapper. I think that solves the naming concerns very well.

Thoughts?

I really like that idea 👍 one fewer new API while still supporting all original use cases. Any good reason not to go that way?

Implemented! All usages of useSetting are refactored to useSettings, and the source folder name is now use-settings.

@tellthemachines This is a change that could be a separate PR. I found that the useSetting( 'layout' ) call can be completely removed, because the usedLayout value is no longer used. Only its .type field is used. And the type can be calculated without knowing defaultLayout. That value always comes from the layout object.

usedLayout was used until #47477 before usages of __experimentalLayout, but today it's redundant.

Ah well spotted! Yes, this can be removed. Not too fussed about whether it's done here or in a separate PR, your choice 😄

Should we add a note right here that it's deprecated in favor of useSettings()?

Note added 👍

Should we keep the EMPTY_OBJECT fallback here?

That's not needed because defaultLayout is used only with an object spread operator, ...defaultLayout, which treats undefined or null as an empty object already.

Would be nice to avoid the mutation here.

I think that local mutation is fine, it's not worse in any way than creating a new array and concatenating existing arrays into it with the spread operator.

There's no need to introduce a bulk settings filter right?

Certainly not, it would be counterproductive. If you want to modify a setting with a plugin, you want to register only one filter, not two of them, both useSetting and useSettings.

I'd keep a test for useSetting() too, even if it's just a single one.

Added a test for useSetting 👍

Not from this PR, but this appears to be so repetitive! I wonder if we should abstract it to a separate hook with these default args.

Yes! The default units change from case to case, but the rest is the same.

Should we get layout.wideSize directly here since that's the only one we need?

Maybe, but I'll keep the existing 'layout' retrieval. The codebase consistently uses 'layout', never its subfields.

Would be nice if we could avoid mutation.

At the same time, I'm not convinced useSetting() really needs to be deprecated. It uses useSettings() under the hood, so what difference does it make if we continue using it when we want to retrieve a single setting?